Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix more windows tests #1581

Closed
wants to merge 6 commits into from
Closed

Fix more windows tests #1581

wants to merge 6 commits into from

Conversation

talregev
Copy link
Contributor

Fix more windows tests

@varunagrawal varunagrawal reopened this Jul 18, 2023
@talregev
Copy link
Contributor Author

talregev commented Jul 18, 2023 •
edited
Loading

gtsam unstable doesn't compile at all at windows, because it has multiple definition error.
Also tests now compile, but they fails because they have exceptions. It mean now we need to change the tests / lib for correctness.
@varunagrawal
Can you take a look of it?

@talregev
Copy link
Contributor Author

talregev commented Jul 18, 2023 •
edited
Loading

From stackoverflow:
https://stackoverflow.com/questions/44960760/msvc-dll-exporting-class-that-inherits-from-template-cause-lnk2005-already-defin

Maybe we can add the flag:
/FORCE:MULTIPLE for windows in cmake.
@varunagrawal Do you want to try that?

https://stackoverflow.com/questions/58008297/how-to-add-compile-flags-in-cmake-on-windows

@talregev
Copy link
Contributor Author

talregev commented Jul 18, 2023 •
edited
Loading

@varunagrawal
I added /FORCE:MULTIPLE for windows for all targets. now also gtsam_unstable is compile, also the gtsam tests.
With this flag, microsoft c++ behave as gcc with templates. It will be more easier to develop like that.
Tests are now failing because of correctness. Can you take care of it?
Also there is a metis library for windows that I mention in previous PR that it will not compile for windows because you take a portion of this library only for linux and osx. You should also fix it.

I can check also for gtsam_unstable tests. But first please review these changes, and comment your thoughts.

@talregev talregev closed this Jul 20, 2023
@dellaert
Copy link
Member

Was this closed because superseded by #1584 ?

@talregev
Copy link
Contributor Author

Was this closed because superseded by #1584 ?

Yes.

@varunagrawal varunagrawal mentioned this pull request Jul 23, 2023
Merged
@talregev talregev deleted the TalR/fix/windows-tests branch July 24, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varunagrawal varunagrawal Awaiting requested review from varunagrawal

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@talregev @dellaert @varunagrawal